Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generic cost_model #3036

Closed
wants to merge 6 commits into from
Closed

Conversation

apfitzge
Copy link

Problem

  • need cost-model to be generic over transaction type so we can use more efficient transaction type

Summary of Changes

  • pass in is_simple_vote_transaction and TransactionSignatureDetails to calculate_cost

Fixes #

Comment on lines +46 to +47
is_simple_vote_transaction: bool,
signatures_count_detail: &TransactionSignatureDetails,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_simple_vote_transaction is not cached on SVMMessage trait, nor do I think we want to add it.

We probably could pass a RuntimeTransaction here instead of transaction and separate fields; but not sure CostModel should be aware of runtime_transaction... @tao-stones any thoughts on best path here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I was aiming to replace Sanitized[Versioned]Transaction with RuntimeTransaction in CostModel, and in banking too, so the cached transaction meta data are easily shared.

I also think SVMMessage trait should not support cache.

Is this still possible that: after receiving packs from sigverify, in the earliest possible point, construct RuntimeTransaction with its static meta. Then passing it through banking pipeline since RuntimeTransaction support SVMMessage, then "cast" back to RuntimeTransaction when need to access meta?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's effectively what I want to get to. I'm just having trouble getting there without changing everything at once.

Maybe I just need to bite the bullet and do that though.
Might be a few more things we can separate out, so I'll try that. but think this cost-model change is probably not one of them!

@apfitzge apfitzge closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants